CMR-11422: Add parameter to only allow validate keyword usage by provider (Enforcement list) - #2490
Conversation
eudoroolivares2016
changed the title
b19d980 to
0e94e36
Compare
eudoroolivares2016
changed the title
| (defn- generate-lookup-by-related-urls-map | ||
| "Create a map with the related url comparison map as keys to the UUID for that related url." | ||
| [gcmd-keywords-map] | ||
| (def gkm gcmd-keywords-map) |
There was a problem hiding this comment.
Please remove debugging code.
| Example \"PROV1,PROV2\", would enforce keyword validation for PROV1 and PROV2. | ||
| If no providers should have keyword validation enforced, set to an empty array." | ||
| {:default [] | ||
| :parser #(map (comp keyword string/trim) (string/split % #","))}) |
There was a problem hiding this comment.
So if the CMR_KEYWORD_ENFORCED_PROVIDERS parameter store does not exist an empty array [] is returned. What if an empty string is defined in the parameter store ""
When the input % is an empty string "", this snippet will evaluate to a list containing a single empty keyword: (:).
Step-by-Step Breakdown
(string/split "" #",")
Splitting an empty string does not return an empty list. It returns a vector containing the empty string itself: [""].
(string/trim "")
Trimming an empty string simply returns the empty string: "".
(keyword "")
Passing an empty string to keyword produces an empty keyword printed as : in Clojure.
Why This Is Dangerous
An empty keyword (:) is valid Clojure syntax, but it is rarely intended.
It can cause silent bugs because:It evaluates to truthy in conditional checks.
It will not match a proper keyword like :provider-id.It consumes a tiny amount of memory in the keyword table that cannot be garbage collected.
;; Option 1: Remove blanks before processing (Recommended)
(->> (string/split % #",")
(remove string/blank?)
(map (comp keyword string/trim)))
;; Input: "" => Output: ()
;; Option 2: Use keep to filter out blanks during the map step
(keep #(when-not (string/blank? %)
(keyword (string/trim %)))
(string/split % #","))
;; Input: "" => Output: ()
eereiter
left a comment
eereiter
left a comment
There was a problem hiding this comment.
I have a couple of small changes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2490 +/- ##
==========================================
- Coverage 57.95% 57.93% -0.03%
==========================================
Files 1074 1074
Lines 74707 74649 -58
Branches 2188 2176 -12
==========================================
- Hits 43294 43245 -49
- Misses 29365 29368 +3
+ Partials 2048 2036 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6662401 to
7a0e83e
Compare
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Provider enforcement is bypassed by the mere presence of cmr-send-kms-metadata-fixer; the new tests even accept the value false. Nothing in get-validation-options authenticates that request header, so an enforced provider can set it and regain the normal validate-keywords=false path. Gate this carveout on trusted KMS caller identity rather than a client-supplied header, and add a spoofing regression.
878dba7 to
0dc5ed0
Compare
| :parser #(->> (string/split % #",") | ||
| (remove string/blank?) | ||
| (map string/trim)) | ||
| }) |
There was a problem hiding this comment.
while I like this syntax, it is not the clojure way to put closing brackets on a new line.
There was a problem hiding this comment.
Ran it with cljfmt it found some other stuff in there
0cc0590 to
6dfcc88
Compare
6dfcc88 to
e7832d2
Compare
6 participants
Overview
What is the objective?
Adding a parameter store to be able to prevent providers in an enforced list e.g. the esdis providers +- to no longer be able to use the
validate-keyword: falseheader which has allowed unsearchable metadata into the systemThere is a temporary carveout for the KMS API itself until we resolve the cache delay issue either by removing the CMR cache entirely or having KMS issue a cache refresh to CMR
CMR-11524.What are the changes?
Added a new parameter store that is a comma sep list, pulled that into the conditional logic of the validate-keyword value, refactoring KMS lookup function.
What areas of the application does this impact?
CMR ingest and metadata validation
Testing
validate-keywordheader. If they do it will always resolve to being true. Ensure that the KMS API if correcting a metadata keyword issue is still able to make sure of it. (SIT only testing possible)https://cmr.sit.earthdata.nasa.gov/kms/metadata_correctioncurl -i -X POST -H "Authorization: mock-echo-system-token" http://localhost:3006/caches/refresh/kmsregenerates all the caches as it does on mainRequired Checklist
Additional Checklist